[WIP] Websocket authentication for preview #1093
Conversation
|
I have pinged Enter-tainer. He will give first review when he schedule a time. |
I haven't read code yet. Does compat mode means the
I cannot remember it clearly. Typst-preview starts from vscode webview and then it goes into browser and finally integrated into tinymist. I guess this explains why we do |
| /// | ||
| /// This function takes an owned `HyperWebsocketStream` to avoid accidental misuse elsewhere before authentication. | ||
| /// | ||
| /// TODO: This is a basic challenge response authentication with nonces. Is there something more standard we could use? |
|
I just remembered that we actually have 2 websockets here(at least in typst-preview). The "control plane" and the "data plane". This exists because in the very first, typst-preview is not a lsp, so it cannot directly receive buffer updates in vscode. Therefore a websocket server is for sending control messages between editor(vscode) and typst-preview binary. And the data plane server send typst document data to browser/webview for previewing. |
By compat I meant The websocket authentication should work fine with the
Right. For I actually have a related question: There are also two websockets in the |
|
Sorry but I completely forget what does typst-preview compat mode do. @Myriad-Dreamin could you please give more input on this? Thanks! From what I understand so far, people should be using
I believe so. typst-preview.nvim use websocat to connect to control plane. This is documented in https://enter-tainer.github.io/typst-preview/editor.html |
Ah, so the neovim part lives in a different repository. Requiring authentication for the control plane websocket is a breaking change for that neovim package then, I guess. How should we handle this? Looking at that code, it also seems to read the server urls to connect to from the As a possible alternative: @SylvanFranklin recently said to be working on using lsp workspace commands to start the preview etc. for neovim. In that case, neovim would no longer need the control plane server. (cc @chomosuke) |
mirroring HTTP digest authentication
So users can infer how to connect to the control plane server when using `typst preview` cli.
That would be cool. Previously we use websocket because we don't have a good editor/server connection. But as typst-preview has been integrated into tinymist, we could use lsp commands to achieve the same thing. |
It is specific to compatible with the old typst-preview extension. They are still in the repository and built per CI and release. |
|
typst-preview.nvim is happy to take this breaking change to the face and upgrade to using lsp commands (or whatever the new way of starting preview is). Can probably make a new plugin called tinymist.nvim and deprecate the old plugin, or just a new version. |
@tmistele We can add an option to enable/disable the authentication. It should be good to continue warn if the authentication is not enabled. I believe some user will finally find and report the warning log in the next few years. Of course, we could also open an issue to some known downstream, like typst-preview.nvim and typst-preview.el (emacs). |
Hey @tmistele I have paused working on this because I wasn't sure about the status of typst-preview.nvim relating to tinymist. |
I would be fine with a configuration option as long as the default is to turn authentication on. That would of course still break downstream packages. But such downstream packages then have a very easy and quick temporary fix (just add a command line flag which turns authentication off). This should give downstream packages a good way to transition (add the command line flag until they added proper authentication support). Does that sound good to you @Myriad-Dreamin ? I agree we should open issues to downstream packages so they're aware. |
thanks for clarification. i think we can safely remove them when it's time. |
tmistele
force-pushed
the
auth
branch
2 times, most recently
from
16e8710 to
754112b
Compare
… plane + minor cleanups
I've now added such a command line parameter to the
I think this should enable packages to temporarily avoid having to do any real work (just adjust a few urls + command lineoptions) while they transition to proper support for authentication. |
matches previous behavior and allows other tools to slightly more easily build a working url (by omitting the 'previewMode' parameter)
for disabling auth in frontend html and in control plane server
| // 1) browsers allow any website to connect to http servers/websockets on localhost | ||
| // 2) on multi-user systems another (potentially untrusted) user can connect to localhost. |
There was a problem hiding this comment.
Is it possible to mitigate (1) by setting relevant cors header or checking referer/origin in request header? Currently I think current auth workflow is somehow way to complex and "homemade". I wonder if there is any better solution for this.
I also seek for other people's solution. But it seems that they don't do authN for websocket. For example, vite relies on websocket to hot reload dev server. And it's a plain websocket connection and there is not bi direction auth.
From what I understand, if we didn't set Access-Control-Allow-Origin: * in our http server, the browser SHOULD forbid access to 127.0.0.1 if the source is not the same. But I remebered that I saw a poc somewhere so i'm not sure what's happening. It might be: (a) I get it wrong, browser did allow that?? (b) the poc http server also runs on 127.0.0.1, so they do run on the same origin? (c) we do set Access-Control-Allow-Origin: * in our code.
There was a problem hiding this comment.
I think CORS etc. do not work for WebSockets, so I think Access-Control-Allow-Origin won't help.
But here's an idea to make the authentication much less complex (so it's less of a problem that it's "homegrown"):
- We generate two secrets, let's call them
client_secretandserver_secret. - The WebSockets servers knows both. And we also tell the client (the frontend html) about both (e.g. through the URL
http://127.0.0.1:port/#client_secret=...&server_secret=...) - On connect, the client sends the
client_secretto the server. The server verifies that it's correct. - The server sends the
server_secretto the client. The client verifies that it's correct.
That's super simple and does "bi-direction auth".
It has worse security properties than a challenge response authentication with nonces, but for our case that's probably okay.
Edit: Actually, I don't like that. It doesn't work well in the case of multi-user systems. Consider this scenario: We start a websocket server on 127.0.0.1:port and open an html frontend in the browser. Now another user on the same system can try to 1) be faster than us to open a server on 127.0.0.1:port 2) wait until the client sends the client_secret 3) quickly shut down their "evil" server on 127.0.0.1:port so that the legitimate server doesn't error when starting (because the port is already in use). 4) Connect to the legitimate server using the legitimate client_secret. That's bad. Of course, the above needs the local attacker to win a race condition. But I don't want to rely on the absence of such race conditions for security.
The more complex authentication that is implemented at the moment doesn't have such problems because the secret itself is never sent over the connection. Only the hash(secret:challenge:nonce) is transmitted. And challenge and nonce are never controlled by the same party (one is chosen by the server, the other by the client).
Such problems also don't arise if we decide that we just don't support multi-user systems. In that case we can just give the client a token that we check on the sever. No bi-directional authentication needed. Makes things much simpler.
I would prefer not to do this, but it's probably mostly okay in practice since multi-user systems with adversarial users are likely rare.
What do you think?
There was a problem hiding this comment.
I think CORS etc. do not work for WebSockets, so I think Access-Control-Allow-Origin won't help.
but websocket connection is upgraded from a http request? if the http request is blocked, i'd assume it would be impossible to establish ws connection
There was a problem hiding this comment.
I'm not an expert but google says that CORS only applies to http responses, not requests. The Websocket connection only involves an upgrade http request, not a response.
There was a problem hiding this comment.
I just did a quick test and cors indeed doesn't apply to websocket. Would it make sense if we check Origin in http header and reject un-trusted hosts?
VSCode now uses an acquireVsCodeApi message to pass arguments. This also simplifies the code around setState/getState.
There was a problem hiding this comment.
The current code looks generally good to me. I left some comments on the code. The following are overall comments:
- Utilizing the
Sec-WebSocket-ProtocolHTTP Header seems like a good practice taken by the kubernetes, See kubernetes/kubernetes's PR 47740. This can replace the client-to-server stage in our authentication. TypstPreviewSerializermay be broken by authentication. The original PR is here, Persist webview preview through vscode restarts Enter-tainer/typst-preview#300.
| secret, | ||
| wsUrl, | ||
| })).toString(); | ||
| vscode.env.openExternal(vscode.Uri.parse(`http://127.0.0.1:${staticServerPort}/#${queryString}`)); |
There was a problem hiding this comment.
can we put the queryString in the query part? It looks a bit strange to format query arguments as a fragment since URLSearchParams is actual a query string builder. I guess this would work:
const uri = vscode.Uri.parse(`http://127.0.0.1:${staticServerPort}`).with({
query: new URLSearchParams({
previewMode: task.mode === "doc" ? "Doc" : "Slide",
secret,
wsUrl,
})).toString()
})This will give uri http://127.0.0.1:123?previewMode=Doc&secret=blabla&wsUrl=http%51blabla
There was a problem hiding this comment.
I think we shouldn't put it in the query part because the query is transmitted to the server as plain text (unlike the hash part which is not transmitted to the server). So, for example, on a multi-user system another user can try to be quicker than us to open a server at http://127.0.0.1:123. Then, if the legitimate user opens the URL http://127.0.0.1:1234?secret=secret-value, the other user learns the value of secret.
There was a problem hiding this comment.
Isn't 123 already used by the server before serving the frontend?
There was a problem hiding this comment.
Probably yes. But I'm not sure if that's 100% guaranteed by the current structure of the code, I'd have to check. There might be a race condition. I would like to avoid having to think about whether such a race condition exists. Using the fragment/hash means it's easier to reason about the security of the code which I think is good.
There was a problem hiding this comment.
The fact is that the (backend) server is always started before the frontend server. If we are really worried about that, we should avoid passing secret on the URL as soon as possible, instead of putting all arguments behind the fragment.
There was a problem hiding this comment.
Okay there might be two servers, and you meant the server that serves the frontend HTML.
There was a problem hiding this comment.
I haven't run the code in local so I may ignore some cases, but it does look strange to pass all arguments via the fragment part. The fragment part is actually used to locate content in the page.
Are you saying the fragment is used to locate content in the page within tinymist, or did you just mean in general? If the concern is that the argument-passing would interfere with other uses of the fragment: It's possible to reset the fragment to be empty during the initial page load, after we have read and parsed it.
I'm also doubt whether we will pass the query arguments to the server even if we put the arguments on the query part.
The query part is definitely sent to the server as part of the GET
$ curl -v "http://127.0.0.1:23625?previewMode=Doc&secret=blabla&wsUrl=http%51blabla" >/dev/null
[snip]
* Connected to 127.0.0.1 (127.0.0.1) port 23625
* using HTTP/1.x
> GET /?previewMode=Doc&secret=blabla&wsUrl=http%51blabla HTTP/1.1
> Host: 127.0.0.1:23625
> User-Agent: curl/8.11.1
> Accept: */*
>
* Request completely sent off
< HTTP/1.1 200 OK
< content-type: text/html
< content-length: 1687085
[snip]
Or did you mean something else?
There was a problem hiding this comment.
Okay there might be two servers, and you meant the server that serves the frontend HTML.
Ah yes, right. Sorry I had missed this comment before sending my previous reply.
There was a problem hiding this comment.
I may have understood the case here, in short it opens URL with a secret safely. In regard to I have little knowledge about it, I would like not to change the current code here. As a developer, I will feel strange at first glance if all arguments are put in the fragment part. Also, if there is any other solution that avoid passing sensitive arguments via the fragment, please let me know.
There was a problem hiding this comment.
As a developer, I will feel strange at first glance if all arguments are put in the fragment part.
I should probably add a comment explaining this. I agree it can be surprising.
Also, if there is any other solution that avoid passing sensitive arguments via the fragment, please let me know.
For VSCode there's the acquireVsCodeApi() message passing mechanism which I'm using. But that doesn't work outside vscode.
| }); | ||
| } | ||
|
|
||
| export function getAuthenticatedSocket(url: string, secret: string, dec: TextDecoder, enc: TextEncoder): Promise<WebSocketAndSubject> { |
There was a problem hiding this comment.
Could we explain the two-stage authentication here? The comment should answer:
- what's the two-stage authentication?
- what's the source of the
secretvalue? How can a developer can pass the source safely from server to client?- For example, "no potentially sensitive data should be in the html itself.", and the potentially sensitive data refers to the
secret.
- For example, "no potentially sensitive data should be in the html itself.", and the potentially sensitive data refers to the
- how safe is the authentication with or without using
wss, i.e. the websocket over HTTPS?
I can understand them by reading code, but it should be better to put down some comments here directly.
crates/tinymist/src/tool/preview.rs
Outdated
| /// Set "unsafe-disable" to disable websocket authentication for the control plane server. Careful: Among other things, this allows any website you visit to use the control plane server. | ||
| /// | ||
| /// This option is only meant to ease the transition to authentication for downstream packages. It will be removed in a future version of tinymist. | ||
| #[clap(long, value_enum, default_value_t=AuthenticationMode::Enable)] |
There was a problem hiding this comment.
If it is enabled by default, will it break the downstream packages silently?
There was a problem hiding this comment.
It will. Should be put a warning message in the log (at least temporarily, for a few releases) to give people a better chance to understand what happened?
There was a problem hiding this comment.
The typst-preview.nvim might have to change their code if this is enabled by default, because they open the preview in neovim (so they construct the URL by themselves).
There was a problem hiding this comment.
So we might disable it by default, to give a window to change things softly. After most of clients upgraded their code, we can change the default strategy then.
There was a problem hiding this comment.
I haven't actually tested it, but I think opening the preview in the browser will actually continue to work in neovim. It looks like they parse the link from the stderr output of tinymist and I've updated that link to include the secret, like this:
[...]
[2025-01-05T05:21:31Z INFO tinymist::tool::preview] Static file server listening on: http://127.0.0.1:23625/#secret=blablabla&previewMode=Doc
[...]
What would break for neovim, however, is connecting to the control plane server. They will have to implement the websocket authentication for that to work (around here in the code I think)
There was a problem hiding this comment.
So we might disable it by default, to give a window to change things softly. After most of clients upgraded their code, we can change the default strategy then.
Just to be clear: This command line argument only affects the control plane server. The data plane server always requires authentication. Is your suggestion to 1) disable auth by default for the control plane server while continuing to require authentication for the data plane server? Or 2) do you suggest to completely disable authentication by default?
If we want to disable auth for a transition window, I'll be in favor of 1). I think the data plane server authentication is only a very minor burden on downstream packages. For example, in neovim I think no change is required for the data plane authentication (see my previous comment). Only the control plane server auth may require some non-trivial work.
| @@ -399,21 +426,24 @@ pub struct HttpServer { | |||
|
|
|||
| /// Create a http server for the previewer. | |||
| pub async fn make_http_server( | |||
| frontend_html: String, | |||
| serve_frontend_html: bool, | |||
There was a problem hiding this comment.
may revert the change. It doesn't harm security to allow customizing frontend HTML.
There was a problem hiding this comment.
The main reason to do it like this is that it makes it easier to reason about the security properties of the code. If the HTML can be customized and I want to convince myself that there is no security issue, I will have to check every single call site. By not allowing to customize the HTML, I just have to check this one function.
There was a problem hiding this comment.
I'm not sure what is bad about customization. We can take some responsibility to secure them instead of not providing the functionality. This is our work.
There was a problem hiding this comment.
Right, I'm not against customization. My point is that, since we currently don't need customization, it's easier to keep everything contained in a single function because it's easier to reason about. If it becomes necessary to add customization, we can of course change that.
There was a problem hiding this comment.
Uh, so we'll finally change it back. I'm not sure why you feel unconfident about the existing code. Less changes means less controversy.
There was a problem hiding this comment.
The existing code is fine. My only reason for this change is to make it easier to review for security (now and with future changes). I think that's worth it, but I'll change it back if you disagree.
That's a nice trick. If I understand correctly, in our case, that would mean the frontend client sends the
I think this actually works with authentication. To test I tried this: I opened a preview in vscode and then ran the "Developer: Reload Window" command. The preview then automatically reloaded. I don't know much about VSCode though, so I'm not 100% sure I tested this correctly. |
Should we believe that there exists a malicious user living on your computer? I remember we were preventing a script in the browser from scanning and connecting to the preview server. They should not be able to start a sever on your computer.
Same here, I just find you were assuming that an attacker can start a sever on your computer, but this sounds not real to me. |
Yes, for most people it's not realistic that an attacker can start a server on their computer. But for some people it's not that unrealistic. For example, some universities have a pool of computers that all students can use. Suppose I'm a student using tinymist on one of these computers. Then another (malicious) student might simultaneously be logged in on the same computer and try to spy on me by starting a server that impersonates tinymist (all users on that computer share the same I agree that such multi-user systems are not very common nowadays. Most people do not share their computer with anyone or only share their computer with people they trust. So if you want, you can decide that you don't want include such scenarios in your security considerations (in your "threat model"). That's probably a reasonable position given that multi-user systems with users that don't trust each other are relatively rare. The current version of this PR, does try to provide security for such "multi-user" systems, however. See e.g. the comments here tinymist/crates/tinymist/src/tool/preview/auth.rs Lines 60 to 61 in 08d4fc7 I did it this way because I think it's good to provide security for such multi-user systems. If you decide to not care about this, a lot of what we discussed above becomes simpler. (For example, there would be no need to authenticate the server to the client. We could pass the secret as part of the query part of the URL. We wouldn't need challenge response authentication etc.). Actually I now suspect we might have been talking past each other a bit because we haven't first agreed what to include in the "threat model". I had included such multi-user systems, you perhaps hadn't. |
Not . I believe when you're editing some document containing sensitive information, you should be in correct environment. If the sensitive information is personal, you should be on your personal computer. If that is owned by some group, the users of the computer should be all benign colleagues in the group, right?
I didn't notice that you further upgraded the threat model a lot more. If we can avoid breaking anything, we can downgrade to the model that only remove the assumption "no script in the browser can scan and connect to the preview server when you are editing preview document." For example, the control-panel server shouldn't be broken by this PR. I feel sorry that I didn't notice it early. |
|
The reason I requested to downgrade the model is not that secure typst-preview on "multi-user systems" be not attractive. It is that we cost too much on a not so important attack surface, especially neovim and emacs people have to tolerate the days their editor plugins haven't follow up our changes. Some of them might also leave typst-preview because of the "bug" that some previous feature is not working. I would also feel worry if we cannot land any improvement after long discussion. In this sense, we can make it smoother to enhance our server step by step. The first step is to authenticate by our owned approach. If we could, we should use the approach that has been verified by some authority or popular applications. This can also be the second step. After that, we can think of some nice way to make it more secure. |
|
I'll think a bit about how to minimize breakage of downstream packages by leaving out multi-user systems for now, but it might take a bit longer now that the holidays are over. |
This adds authentication so that users don't have to worry about a) third parties being able to connect to the tinymist servers on 127.0.0.1 (browsers allow any website to do that) and b) third parties impersonating the tinymist server, potentially making the frontend html/javascript leak information (e.g. on multi-user systems).
This is not ready yet. But I thought I'd post it so you can comment on the overall approach.
This seems to work for me but I haven't tested everything. Especially the VSCode extension I have tested only very superficially. And I have not at all tested the "compat" integration with
typst-preview.Some random notes and questions:
typst-previewwill not be updated to add support for authentication? For now, I've built in an escape hatch to avoid authentication for the "compat" preview mode (completely untested!). How do you think this should be handled? It would be easiest to just not support thetypst-previewcompat mode but I guess you don't want to do that yet?replace(...)ing ports and user settings directly into the html. These settings are now passed in as part of URL that's opened in the browser (likehttp://127.0.0.1:23625/#param=value). That code restructuring makes it more immediately obvoius that there's nothing interesting in the html for an attacker.(
Unfortunately, the vscode webview panel doesn't seem to support setting a location hash (or maybe it does and I didn't figure out how?), so I've built in an escape hatch for the VSCode webview panel where the settings are nowFound a better way, by using thereplace(...)ed into the html. That's somewhat unfortunate, but not a problem from a security point of view because the vscode webview panel cannot be accessed from outside vscode I think.acquireVsCodeApimessage passing mechanism)typst-webview-assets/in the frontend html. That didn't seem to have any effect in my tests. Does this code ever do anything?cc @Myriad-Dreamin